Codecov Report

Merging #901 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #901    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        32     32            
  Lines      3508   3513     +5     
====================================
+ Hits       3388   3393     +5     
  Misses      120    120

Thanks for sending this in! Great work on this one! 😄 Just minor review comments.

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 238 at r1 (raw file):

$script:localizedData.NotActiveClusterNode

This resource has not yet been localized so we can't use this method (there are no localized string file). We have to hard code the string.

Like here:

https://github.com/PowerShell/xSQLServer/blob/1eddecbc424aec7b5c77eec70309cc8f88e10c7b/DSCResources/MSFT_xSQLServerAlwaysOnAvailabilityGroup/MSFT_xSQLServerAlwaysOnAvailabilityGroup.psm1#L625

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 358 at r1 (raw file):

This is used with -TestCases parameter (Or I'm I wrong?). So maybe we need to set this the correct value if we are not using test cases? See below for example. But you could just hard code the correct value into the string if you don't want to use test cases.

https://github.com/PowerShell/xSQLServer/blob/1eddecbc424aec7b5c77eec70309cc8f88e10c7b/Tests/Unit/MSFT_xSQLServerAlwaysOnAvailabilityGroup.Tests.ps1#L1362-L1371

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 372 at r1 (raw file):

                }

                It 'Should be "true" when ProcessOnlyOnActiveNode is <mockProcessOnlyOnActiveNodeOriginal>.' {

Same as previous comment.

Comments from Reviewable

Review status: 4 of 6 files reviewed at latest revision, 3 unresolved discussions.

DSCResources/MSFT_xSQLServerMaxDop/MSFT_xSQLServerMaxDop.psm1, line 238 at r1 (raw file):

Done.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 358 at r1 (raw file):

I changed it so let me know if it is more what you were thinking.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 372 at r1 (raw file):

I changed it so let me know if it is more what you were thinking.

Comments from Reviewable

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 306 at r2 (raw file):

'When the system is not in the desired state and Ensure is set to absent and ProcessOnlyOnActiveNode is set to true'

This test actually mimics the previous test, that it should actually fail (return $false) because MaxDop has the wrong value (see It-block description in previous test), but because ProcessOnlyOnActiveNode is set to $true in this test, the test correctly returns $true. Maybe we should change the description here, or in the below It-block, to make that more clear?

Comments from Reviewable

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 306 at r2 (raw file):

I changed the description and the It-Block verbiage

Comments from Reviewable

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.

Tests/Unit/MSFT_xSQLServerMaxDop.Tests.ps1, line 306 at r2 (raw file):

Done.

Comments from Reviewable

Tests looks good. Thanks! Awesome work! 😄

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable